fix(skills): harden UAT debug-bundle triage against malformed input by njhensley · Pull Request #1914 · NVIDIA/aicr

njhensley · 2026-07-24T20:42:47Z

Summary

Addresses the three CodeRabbit findings on #1913: a crash in the triage digest, an unsanitized download path, and a directory collision when a run carries more than one debug bundle.

Motivation / Context

All three were raised on #1913 after it merged. The first is a genuine defect that defeats the feature's own error-handling contract; the other two are latent.

Fixes: N/A
Related: #1913

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Refactoring (no functional changes)
Build/CI/tooling

Component(s) Affected

CLI (cmd/aicr, pkg/cli)
API server (cmd/aicrd, pkg/server)
Recipe engine / data (pkg/recipe)
Bundlers (pkg/bundler, pkg/component/*)
Collectors / snapshotter (pkg/collector, pkg/snapshotter)
Validator (pkg/validator)
Core libraries (pkg/errors, pkg/k8s)
Docs/examples (docs/, examples/)
Other: agent skills (.agents/skills/aicr-uat-report/)

Implementation Notes

1. TypeError aborted the download loop (real defect). report.json with an explicit "tests": null made .get("tests", []) return None — a default only applies when the key is absent — so iteration raised TypeError, which the except (OSError, ValueError, AttributeError) clause did not list. triage_digest is called per run with no surrounding guard, so one truncated report killed every remaining run. That directly contradicts the contract #1913 introduced, where a bad artifact is reported per run and processing continues.

Reproduced against the merged code before fixing:

PRE-FIX CRASH CONFIRMED: TypeError: 'NoneType' object is not iterable

Fixed with or fallbacks for null-valued keys plus TypeError in the except tuple. A malformed report now degrades to report.json present but unparseable for that one run.

2. Unsanitized path components. Reservation tokens reach the script via the workflow run-name, and NEW_TITLE/OLD_TITLE accept any non-whitespace token, so svc/gpu are not guaranteed well-formed before being joined onto the output directory. New slug() collapses everything outside [a-z0-9-], which removes separators and dot runs together. Reaching this requires write access to dispatch the workflow, so it is defense in depth rather than a live exposure — but it is one line and the destination is attacker-influenced in principle.

3. Multi-artifact collision. A run carrying more than one debug bundle unpacked them into a shared directory, merging two clusters' state so the printed digest described neither. Each artifact now gets its own subdirectory once there is more than one; the single-bundle path (every run observed so far) is unchanged.

Also corrected the debug_artifacts docstring, which claimed to filter expired artifacts and sort newest-first. It does neither — the caller filters deliberately, so that "never uploaded" and "aged out" stay distinguishable.

Testing

python3 .agents/skills/aicr-uat-report/uat_report.py --days 2 \
  --download-debug /tmp/uat-debug2 --max-downloads 2

Live regression run: green, output unchanged from #1913 for the normal path (one bundle fetched, one "no artifact" correctly reported).

Targeted cases, all exercised directly against triage_digest and slug:

Input	Result
`{"results": {"tests": null}}`	`0/0 checks failing` (crashed pre-fix)
`{"results": null}`	`0/0 checks failing`
`[1,2,3]` (top-level list)	`present but unparseable`
truncated JSON	`present but unparseable`
valid report	`1/1 checks failing: x(failed)`
`slug("EKS","GB200","training",id)`	`eks-gb200-training-<id>` — unchanged
`slug("../../etc","H100",…)`	`etc-h100-…`, stays under the output dir
`slug("AWS/../..","..",…)`	`aws----inference-2`, stays under the output dir
`slug("...","","","")`	`unknown`

No exception escaped in any case; every traversal attempt resolved inside the download directory.

make qualify not run — no Go, YAML, or build inputs changed; verified with py_compile plus the runs above.

Risk Assessment

Low — Isolated change, well-tested, easy to revert
Medium — Touches multiple components or has broader impact
High — Breaking change, affects critical paths, or complex rollout

Rollout notes: Agent tooling only. Behaviour on well-formed input is identical to #1913; the changes only affect malformed-input and multi-artifact paths.

Checklist

Tests pass locally (make test with -race) — N/A, no Go changes; see Testing
Linter passes (make lint) — N/A, no Go/YAML changes; py_compile clean
I did not skip/disable tests to make CI green
I added/updated tests for new functionality — N/A, skills have no test harness; cases exercised directly, table above
I updated docs if user-facing behavior changed — no user-visible behaviour change
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S)

Review follow-ups from NVIDIA#1913. triage_digest crashed the whole --download-debug loop on a report.json carrying an explicit "tests": null — .get("tests", []) returns None only when the key is absent, so iteration raised TypeError, which the except clause did not list. Confirmed against the merged code. Use `or` fallbacks for null-valued keys and catch TypeError, so one truncated report degrades to "unparseable" for that run instead of aborting the remaining runs. Sanitize the download directory name. Reservation tokens reach the script through the workflow run-name and the title regexes accept any non-whitespace token, so svc/gpu are not guaranteed well-formed; collapse everything outside [a-z0-9-] to strip separators and dot runs before joining onto the output directory. Requires repo write access to reach, so this is defense in depth rather than a live exposure. Give each artifact its own subdirectory when a run carries more than one debug bundle — a shared directory merged two clusters' state and left the digest describing neither. Also correct the debug_artifacts docstring: it returns expired artifacts too, and never sorted. Signed-off-by: Nathan Hensley <nhensley@nvidia.com>

coderabbitai · 2026-07-24T20:48:14Z

📝 Walkthrough

Walkthrough

The UAT report workflow now includes expired debug artifacts in listings and adds filesystem-safe directory naming. Multiple active artifacts for a run are downloaded into separate directories. Triage parsing treats null results data as empty and catches TypeError for malformed reports. Digest generation and output now use each artifact’s dedicated directory.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

NVIDIA/aicr#1913: Modifies the same UAT debug-download and triage workflow.

Suggested reviewers: mchmarny

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the main change: hardening UAT debug-bundle triage against malformed input.
Description check	✅ Passed	The description accurately summarizes the fixes and is clearly related to the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.agents/skills/aicr-uat-report/uat_report.py:
- Around line 123-133: Add type annotations to the variadic parts parameter and
return value of slug, using types compatible with the existing str conversion
and filesystem-safe string result.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 3bb32642-afaa-4520-9036-4616dfebc708

📥 Commits

Reviewing files that changed from the base of the PR and between 7e0f4a1 and 8dc352b.

📒 Files selected for processing (1)

.agents/skills/aicr-uat-report/uat_report.py

coderabbitai · 2026-07-24T20:48:17Z

+def slug(*parts):
+    """Build a filesystem-safe directory name from run metadata.
+
+    Reservation names reach us through the workflow's run-name, and the title
+    regexes accept any non-whitespace token, so `svc`/`gpu` are not guaranteed
+    to be well-formed. Collapsing everything outside [a-z0-9-] removes both
+    separators and dot runs, so no component can traverse out of the download
+    directory.
+    """
+    raw = "-".join(str(p) for p in parts).lower()
+    return re.sub(r"[^a-z0-9-]+", "-", raw).strip("-") or "unknown"


📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add annotations to slug.

Ruff reports ANN002 for the untyped variadic parameter. Add parameter and return annotations.

Suggested fix

-def slug(*parts): +def slug(*parts: object) -> str:

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

def slug(*parts):

"""Build a filesystem-safe directory name from run metadata.

Reservation names reach us through the workflow's run-name, and the title

regexes accept any non-whitespace token, so `svc`/`gpu` are not guaranteed

to be well-formed. Collapsing everything outside [a-z0-9-] removes both

separators and dot runs, so no component can traverse out of the download

directory.

"""

raw = "-".join(str(p) for p in parts).lower()

return re.sub(r"[^a-z0-9-]+", "-", raw).strip("-") or "unknown"

def slug(*parts: object) -> str:

"""Build a filesystem-safe directory name from run metadata.

Reservation names reach us through the workflow's run-name, and the title

regexes accept any non-whitespace token, so `svc`/`gpu` are not guaranteed

to be well-formed. Collapsing everything outside [a-z0-9-] removes both

separators and dot runs, so no component can traverse out of the download

directory.

"""

raw = "-".join(str(p) for p in parts).lower()

return re.sub(r"[^a-z0-9-]+", "-", raw).strip("-") or "unknown"

🧰 Tools

🪛 Ruff (0.15.21)

[warning] 123-123: Missing type annotation for *parts

(ANN002)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/aicr-uat-report/uat_report.py around lines 123 - 133, Add type annotations to the variadic parts parameter and return value of slug, using types compatible with the existing str conversion and filesystem-safe string result.

Source: Linters/SAST tools

yuanchen8911

/lgtm

…alth checks Closes NVIDIA#1246. - kueue: resourceflavors/clusterqueues/localqueues.kueue.x-k8s.io (verified via in-tree manifests) - kubeflow-trainer: trainjobs.trainer.kubeflow.org (verified via tests/chainsaw/ai-conformance/kind-training-kubeflow/assert-crds.yaml) - network-operator: nicclusterpolicies.mellanox.com (verified via components/network-operator/manifests/nic-cluster-policy-aks.yaml) - k8s-nim-operator: nimservices/nimcaches/nimpipelines.apps.nvidia.com (verified via docs/conformance/cncf/v1.35/nim-eks/evidence/robust-operator.md) - gatekeeper: constrainttemplates.templates.gatekeeper.sh + configs.config.gatekeeper.sh (needs live-cluster confirmation against chart 3.22.2, flagged inline) - slinky-slurm-operator: negative finding documented - crds.enabled: false, CRDs owned entirely by sibling slinky-slurm-operator-crds chart Signed-off-by: Mohit Yadav <ymohit799057@gmail.com> docs: restore slinky-slurm-operator health check, add comment-only negative finding Signed-off-by: Mohit Yadav <ymohit799057@gmail.com> fix: address review comments on NVIDIA#1246 CRD backfill fix: sync kubeflow-trainer CRD docs with asserted trainingruntimes (coderabbitai) address review: soften gatekeeper verification wording, fix volatile line refs, rename CRD step for consistency chore: pausing scans chore: skills cleanup chore(deps): Update python Docker tag to v3.14 (NVIDIA#1906) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com> revert(validators): pin aiperf-bench base back to python:3.13-slim (NVIDIA#1909) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> fix(rekor-monitor): correlate identity via release-workflow run history (NVIDIA#1903) Signed-off-by: Brian Lockwood <lockwobr@gmail.com> Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com> fix(validators): multi-stage aiperf-bench build; bump aiperf to 0.11.0 (NVIDIA#1912) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> feat(skills): download UAT debug bundles for failure triage (NVIDIA#1913) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> chore: deps: bump renovatebot/github-action from 46.1.20 to 46.1.21 (NVIDIA#1923) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> feat(skills): add aicr-triage skill for project board triage (NVIDIA#1911) chore(deps): regenerate stale THIRD_PARTY_NOTICES.md (NVIDIA#1926) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> feat(server): non-interactive bundle attestation for /v1/bundle (NVIDIA#1891) chore(deps): Update dependency awscli to v1.45.52 (NVIDIA#1905) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com> Co-authored-by: Nathan Hensley <229213852+njhensley@users.noreply.github.com> ci(stale): drop phantom priority/critical exempt label (NVIDIA#1925) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> chore(deps): Update dependency sigstore/cosign to v3.1.2 (NVIDIA#1924) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Lalit Adithya <ladithyav@nvidia.com> Co-authored-by: Brian Lockwood <lockwobr@gmail.com> feat(skills): add aicr-cross-review skill for multi-agent PR review (NVIDIA#1915) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> chore: deps: bump github.com/prometheus/client_golang from 1.24.0 to 1.24.1 (NVIDIA#1922) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lalit Adithya <ladithyav@nvidia.com> Co-authored-by: Brian Lockwood <lockwobr@gmail.com> fix: resolve goconst and errorlint warnings across OCI, coverage, validators (NVIDIA#1916) Signed-off-by: Andrew White <andrewh@cdw.com> chore(deps): Update testing-tools (NVIDIA#1934) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> fix(skills): harden UAT debug-bundle triage against malformed input (NVIDIA#1914) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> chore: deps: bump actions/stale from 10.4.0 to 11.0.0 (NVIDIA#1931) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lalit Adithya <ladithyav@nvidia.com> fix(ci): sync coverage docs to 80% and fail closed on bad threshold (NVIDIA#1937) docs: correct issue/PR lifecycle table, fix reminder dedupe (NVIDIA#1921) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> Co-authored-by: Nathan Hensley <229213852+njhensley@users.noreply.github.com> chore: deps: bump actions/checkout from 7.0.0 to 7.0.1 (NVIDIA#1930) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Nathan Hensley <229213852+njhensley@users.noreply.github.com> Co-authored-by: Brian Lockwood <lockwobr@gmail.com> fix(recipe,client): reject duplicate ComponentRef names, omit disable… (NVIDIA#1917) Co-authored-by: Nathan Hensley <229213852+njhensley@users.noreply.github.com> fix(validators): pass aiperf model via --model flag (NVIDIA#1939) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> fix(skills): recover cross-review lanes, harden consensus/fallback (NVIDIA#1932) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> feat(rekor-monitor): resumable identity scan to catch up large backlogs (NVIDIA#1929) Signed-off-by: Brian Lockwood <lockwobr@gmail.com> fix(bundler): create argocd-helm static/ only when populated (NVIDIA#1944) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> chore: deps: update hashicorp/azurerm requirement from ~> 4.0 to ~> 5.0 in /infra/uat-azure-account (NVIDIA#1945) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> fix(ci): run validators/ unit tests in make test, exclude from coverage gate (NVIDIA#1918) Signed-off-by: Mohit Yadav <ymohit799057@gmail.com> Co-authored-by: Nathan Hensley <229213852+njhensley@users.noreply.github.com> feat(corroborate): improve evidence dashboard UX (NVIDIA#1935) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> fix(api): accept versionless legacy bundle recipes (NVIDIA#1943) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> test(bundler): cover argocd-helm OCP bundling and pin its error contract (NVIDIA#1950) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> chore: remove stray MOFED debug artifacts from repo root (NVIDIA#1955) Signed-off-by: Nathan Hensley <nhensley@nvidia.com> fix(deps): bump network-operator default to v26.4.1 (NVIDIA#1938) Signed-off-by: Atif Mahmood <atif1996@users.noreply.github.com> Co-authored-by: Atif Mahmood <atif1996@users.noreply.github.com> chore: deps: bump renovatebot/github-action from 46.1.21 to 46.2.0 (NVIDIA#1964) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> chore(deps): Update ministackorg/ministack Docker tag to v1.4.7 (NVIDIA#1965) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Lalit Adithya <ladithyav@nvidia.com> feat(recipe): implement the ADR-015 profile core (NVIDIA#1933) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> fix(bom): extract nested operator images (NVIDIA#1960) Signed-off-by: Yuan Chen <yuanchen97@gmail.com> address yuanchen8911 review: bump network-operator to 26.4.1 with nicnodepolicies exclusion, close gatekeeper TODO with verification

njhensley requested a review from a team as a code owner July 24, 2026 20:42

njhensley self-assigned this Jul 24, 2026

github-actions Bot added the size/S label Jul 24, 2026

njhensley added the theme/ci-dx CI pipelines, developer experience, and build tooling label Jul 24, 2026

coderabbitai Bot reviewed Jul 24, 2026

View reviewed changes

njhensley mentioned this pull request Jul 27, 2026

UAT debug artifacts collide on re-run: attempt 2 upload fails, triage reads stale attempt-1 bundle #1928

Open

yuanchen8911 self-requested a review July 28, 2026 16:14

yuanchen8911 approved these changes Jul 28, 2026

View reviewed changes

Merge branch 'main' into fix/uat-report-triage-robustness

584882f

njhensley enabled auto-merge (squash) July 28, 2026 17:02

njhensley merged commit 2abbd2b into NVIDIA:main Jul 28, 2026
36 checks passed

njhensley deleted the fix/uat-report-triage-robustness branch July 28, 2026 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(skills): harden UAT debug-bundle triage against malformed input - #1914

fix(skills): harden UAT debug-bundle triage against malformed input#1914
njhensley merged 2 commits into
NVIDIA:mainfrom
njhensley:fix/uat-report-triage-robustness

njhensley commented Jul 24, 2026

Uh oh!

coderabbitai Bot commented Jul 24, 2026

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 24, 2026

Uh oh!

yuanchen8911 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

njhensley commented Jul 24, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

coderabbitai Bot commented Jul 24, 2026

Walkthrough

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

yuanchen8911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants